Skip to content

Fix Callback bugs #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Fix Callback bugs #39

merged 2 commits into from
Nov 8, 2023

Conversation

alrvid
Copy link
Contributor

@alrvid alrvid commented Nov 7, 2023

  1. Fixes MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL redefine bug

The redefines break paragraph 3.2.4 of the C++14 specification (a subcategory of
the ODR - One Definition Rule) in an insidious way. There's no diagnostic
required in these cases, which means no warnings, no errors required by the
compiler. It's also undefined behavior. There's a conditional compilation of the
function call_fn() in the Callback implementation, and in practice, it's inlined
in the call() function in the same object file. Depending on the
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL setting you get an extra "ldr r3,[r3, #0]"
instruction or not inlined in call(). Without that indirection, calling a
nontrivial callback leads to the execution of "bx r3", where r3 contains the
address of the ops structure (dynamically dispatched operations), instead of the
address contained in the first member of that structure, which would lead to the
type-erased function pointer. And because the structure is at an even address,
there's a UsageFault when the CPU tries to enter Arm mode, which isn't supported
on Cortex-M. When there's a single definition of
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL everything is fine, because there's only
one definition of call_fn() and call() - the last function of the two is the
only one that exists in practice, in the binary. But when we redefine
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL we get two definitions of the call()
function at the same time, breaking paragraph 3.2.4. And remember, no diagnostic
required, and this is undefined behavior. So anything goes. On macOS,
USBDevice::endpoint_add() is then linked against call() from USBHost.o,
which contains the extra "ldr r3,[r3, #0]" instruction. On Windows, it's instead
linked against the call() function from USBHostSerial.o, which contains no
indirection, and it leads to a crash. These two translation units are compiled
with different settings because the redefines are applied to some parts of the
library but not all of them. Remember, breaking 3.2.4 is undefined behavior, so
it's ok to link this at random. But keeping them equal wouldn't be ok anyway
because 3.2.4 would still be broken by the library in combination with the core.

  1. Fixes missing default Mbed configuration

Parts of the library don't know about the default Mbed configuration. If only
the redefines are removed, Callback.h is included already in USBHostConf.h,
but without any default Mbed configuration, so we get the trivial version of
call(). Next, Arduino.h is included, which includes the Mbed configuration
indirectly, but the damage has already been done. Over in USBHostSerial.h,
USBHostConf.h is included. After that, Callback.h is included in
USBHostSerial.h, and we have the correct Mbed configuration in place, but
since it's already been included once, the correct version of call() is never
actually used. So we get two versions of call() again, which is undefined
behavior, and a crash.

Alrik Vidstrom added 2 commits November 3, 2023 11:19
The redefines break paragraph 3.2.4 of the C++14 specification (a subcategory of
the ODR - One Definition Rule) in an insidious way. There's no diagnostic
required in these cases, which means no warnings, no errors required by the
compiler. It's also undefined behavior. There's a conditional compilation of the
function call_fn() in the Callback implementation, and in practice, it's inlined
in the call() function in the same object file. Depending on the
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL setting you get an extra "ldr r3,[r3, #0]"
instruction or not inlined in call(). Without that indirection, calling a
nontrivial callback leads to the execution of "bx r3", where r3 contains the
address of the ops structure (dynamically dispatched operations), instead of the
address contained in the first member of that structure, which would lead to the
type-erased function pointer. And because the structure is at an even address,
there's a UsageFault when the CPU tries to enter Arm mode, which isn't supported
on Cortex-M. When there's a single definition of
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL everything is fine, because there's only
one definition of call_fn() and call() - the last function of the two is the
only one that exists in practice, in the binary. But when we redefine
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL we get two definitions of the call()
function at the same time, breaking paragraph 3.2.4. And remember, no diagnostic
required, and this is undefined behavior. So anything goes. On macOS,
USBDevice::endpoint_add() is then linked against call() from USBHost.o,
which contains the extra "ldr r3,[r3, #0]" instruction. On Windows, it's instead
linked against the call() function from USBHostSerial.o, which contains no
indirection, and it leads to a crash. These two translation units are compiled
with different settings because the redefines are applied to some parts of the
library but not all of them. Remember, breaking 3.2.4 is undefined behavior, so
it's ok to link this at random. But keeping them equal wouldn't be ok anyway
because 3.2.4 would still be broken by the library in combination with the core.
Parts of the library don't know about the default Mbed configuration. If only
the redefines are removed, Callback.h is included already in USBHostConf.h,
but without any default Mbed configuration, so we get the trivial version of
call(). Next, Arduino.h is included, which includes the Mbed configuration
indirectly, but the damage has already been done. Over in USBHostSerial.h,
USBHostConf.h is included. After that, Callback.h is included in
USBHostSerial.h, and we have the correct Mbed configuration in place, but
since it's already been included once, the correct version of call() is never
actually used. So we get two versions of call() again, which is undefined
behavior, and a crash.
@alrvid alrvid requested a review from sebromero November 7, 2023 14:18
Copy link

github-actions bot commented Nov 7, 2023

Memory usage change @ c409e1f

Board flash % RAM for global variables %
arduino:mbed_giga:giga 🔺 +88 - +152 0.0 - +0.01 0 - 0 0.0 - 0.0
arduino:mbed_opta:opta 🔺 +88 - +152 0.0 - +0.01 0 - 0 0.0 - 0.0
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A
Click for full report table
Board examples/DirList
flash
% examples/DirList
RAM for global variables
% examples/FileRead
flash
% examples/FileRead
RAM for global variables
% examples/FileWrite
flash
% examples/FileWrite
RAM for global variables
% examples/OptaDirList
flash
% examples/OptaDirList
RAM for global variables
% examples/PortentaOTA
flash
% examples/PortentaOTA
RAM for global variables
%
arduino:mbed_giga:giga 88 0.0 0 0.0 152 0.01 0 0.0 88 0.0 0 0.0 88 0.0 0 0.0 152 0.01 0 0.0
arduino:mbed_opta:opta 88 0.0 0 0.0 152 0.01 0 0.0 88 0.0 0 0.0 152 0.01 0 0.0 88 0.0 0 0.0
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
Click for full report CSV
Board,examples/DirList<br>flash,%,examples/DirList<br>RAM for global variables,%,examples/FileRead<br>flash,%,examples/FileRead<br>RAM for global variables,%,examples/FileWrite<br>flash,%,examples/FileWrite<br>RAM for global variables,%,examples/OptaDirList<br>flash,%,examples/OptaDirList<br>RAM for global variables,%,examples/PortentaOTA<br>flash,%,examples/PortentaOTA<br>RAM for global variables,%
arduino:mbed_giga:giga,88,0.0,0,0.0,152,0.01,0,0.0,88,0.0,0,0.0,88,0.0,0,0.0,152,0.01,0,0.0
arduino:mbed_opta:opta,88,0.0,0,0.0,152,0.01,0,0.0,88,0.0,0,0.0,152,0.01,0,0.0,88,0.0,0,0.0
arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A

Copy link
Contributor

@sebromero sebromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

@sebromero sebromero merged commit 9210d7d into arduino-libraries:main Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants